distribute framework: add planner #46395

GMHDBJD · 2023-08-24T07:39:30Z

What problem does this PR solve?

Issue Number: ref #46258

Problem Summary:

What is changed and how it works?

logical plan -> task meta -> physical plan -> subtaskmetas -> operators.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

GMHDBJD · 2023-08-24T07:39:37Z

/hold wip

tiprow · 2023-08-24T07:40:10Z

Hi @GMHDBJD. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

codecov · 2023-08-24T07:50:21Z

Codecov Report

Merging #46395 (e51275b) into master (1901239) will decrease coverage by 0.5213%.
Report is 7 commits behind head on master.
The diff coverage is 56.9230%.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #46395        +/-   ##
================================================
- Coverage   73.3422%   72.8209%   -0.5213%     
================================================
  Files          1310       1335        +25     
  Lines        395127     403506      +8379     
================================================
+ Hits         289795     293837      +4042     
- Misses        86876      91197      +4321     
- Partials      18456      18472        +16

Flag	Coverage Δ
integration	`25.7357% <0.0000%> (?)`
unit	`73.5263% <56.9230%> (+0.1840%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
dumpling	`54.0444% <ø> (ø)`
parser	`84.9628% <ø> (+0.0108%)`	⬆️
br	`48.1094% <ø> (-4.2312%)`	⬇️

GMHDBJD · 2023-08-29T07:53:22Z

/unhold

GMHDBJD · 2023-08-29T08:24:48Z

/retest

tiprow · 2023-08-29T08:25:21Z

@GMHDBJD: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

GMHDBJD · 2023-08-29T08:27:34Z

/ok-to-test

GMHDBJD · 2023-08-29T09:11:19Z

/test tiprow_fast_test

ti-chi-bot · 2023-08-29T09:11:21Z

@GMHDBJD: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test build
/test canary-scan-security
/test check-dev
/test check-dev2
/test mysql-test
/test pingcap/tidb/canary_ghpr_unit_test
/test pull-br-integration-test
/test pull-common-test
/test pull-e2e-test
/test pull-integration-common-test
/test pull-integration-copr-test
/test pull-integration-ddl-test
/test pull-integration-jdbc-test
/test pull-integration-mysql-test
/test pull-integration-prisma-test
/test pull-mysql-connector-test
/test pull-sqllogic-test
/test pull-tiflash-test
/test unit-test

The following commands are available to trigger optional jobs:

/test pull-notify-when-compatibility-sections-changed

Use /test all to run the following jobs that were automatically triggered:

pingcap/tidb/ghpr_build
pingcap/tidb/ghpr_check
pingcap/tidb/ghpr_check2
pingcap/tidb/ghpr_mysql_test
pingcap/tidb/ghpr_unit_test

In response to this:

/test tiprow_fast_test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

ywqzzy · 2023-08-29T09:34:04Z

/cc @ywqzzy

ywqzzy · 2023-08-29T09:38:00Z

disttask/importinto/dispatcher.go

+	if err != nil {
+		return nil, err
+	}
+	gTask.Step = nextStep


We can do gtask.Step++ in dispatcher after calling processNormalFlow.
Anyway, this refinement can be done later.

ywqzzy · 2023-08-29T09:40:04Z

disttask/importinto/planner.go

+	physicalPlan := &planner.PhysicalPlan{}
+	inputStreams := make([]planner.StreamSpec, 0)
+	// physical plan only needs to be generated once.
+	// However, our current implementation requires generating it for each step.


How can we generate the physical plan once? Next pr?

if we support pass result from one step two another step, then we only need generated the initial physical plan, all runtime arguments should be define in input stream and generate in executor instead

ywqzzy · 2023-08-29T09:44:41Z

disttask/importinto/planner.go

+	JobID             int64
+	Plan              importer.Plan
+	Stmt              string
+	EligibleInstances []*infosync.ServerInfo


Seems useless？

tidb/disttask/importinto/job.go

Line 185 in 2564090

EligibleInstances: instances,

ywqzzy · 2023-08-29T10:01:32Z

disttask/framework/planner/plan.go

+
+	// PreviousSubtaskMetas is a list of subtask metas from previous step.
+	// We can remove this field if we find a better way to pass the result between steps.
+	PreviousSubtaskMetas [][]byte


hold task_table as member?

hold task_table as member?

The planner in tidb runtime also fetch meta data from tikv. So I think it's ok.

Using PreviousSubtaskMetas is to express that PreviousSubtaskMetas is the result of the previous step. If we use task_table, our planner will be coupled with the system table again, although adding a new interface would be more convenient. 🤔

disttask/framework/planner/plan.go

ywqzzy · 2023-08-31T06:09:17Z

disttask/framework/planner/plan.go

+	Streams []StreamSpec
+}
+
+// StreamSpec is the specification of a stream.


The name is a little bit confusing, can we have a better name?

ywqzzy · 2023-08-31T06:10:23Z

disttask/framework/planner/planner_test.go

+
+type mockPlan struct{}
+
+func (mockPlan) ToTaskMeta() ([]byte, error) {


Maybe we can use gen_mock introduced in #46392

ywqzzy · 2023-08-31T06:11:49Z

disttask/framework/planner/planner_test.go

+	store := testkit.CreateMockStore(t)
+	gtk := testkit.NewTestKit(t, store)


Seems useless? Bootstrap a testkit cost some resource, better not add it.

we need testkit to submit a global task like what we do in handle_test

tidb/disttask/framework/handle/handle_test.go

Line 34 in 7e66761

gtk := testkit.NewTestKit(t, store)

ywqzzy · 2023-08-31T06:13:34Z

disttask/framework/planner/plan.go

+}
+
+// PhysicalPlan is a DAG of processors in distribute framework.
+type PhysicalPlan struct {


Can we add more detailed comments to show the arch of the planner?

GMHDBJD · 2023-08-31T09:35:03Z

/retest

ywqzzy

LGTM

GMHDBJD · 2023-08-31T13:25:01Z

/retest

GMHDBJD · 2023-08-31T13:54:26Z

/test tiprow_fast_test

ti-chi-bot · 2023-08-31T13:54:29Z

@GMHDBJD: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test build
/test canary-scan-security
/test check-dev
/test check-dev2
/test mysql-test
/test pingcap/tidb/canary_ghpr_unit_test
/test pull-br-integration-test
/test pull-common-test
/test pull-e2e-test
/test pull-integration-common-test
/test pull-integration-copr-test
/test pull-integration-ddl-test
/test pull-integration-jdbc-test
/test pull-integration-mysql-test
/test pull-integration-prisma-test
/test pull-mysql-connector-test
/test pull-sqllogic-test
/test pull-tiflash-test
/test unit-test

The following commands are available to trigger optional jobs:

/test pull-notify-when-compatibility-sections-changed

Use /test all to run the following jobs that were automatically triggered:

pingcap/tidb/ghpr_build
pingcap/tidb/ghpr_check
pingcap/tidb/ghpr_check2
pingcap/tidb/ghpr_mysql_test
pingcap/tidb/ghpr_unit_test

In response to this:

/test tiprow_fast_test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

GMHDBJD · 2023-08-31T17:45:05Z

/test tiprow_fast_test

ti-chi-bot · 2023-08-31T17:45:08Z

@GMHDBJD: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test build
/test canary-scan-security
/test check-dev
/test check-dev2
/test mysql-test
/test pingcap/tidb/canary_ghpr_unit_test
/test pull-br-integration-test
/test pull-common-test
/test pull-e2e-test
/test pull-integration-common-test
/test pull-integration-copr-test
/test pull-integration-ddl-test
/test pull-integration-jdbc-test
/test pull-integration-mysql-test
/test pull-integration-prisma-test
/test pull-mysql-connector-test
/test pull-sqllogic-test
/test pull-tiflash-test
/test unit-test

The following commands are available to trigger optional jobs:

/test pull-notify-when-compatibility-sections-changed

Use /test all to run the following jobs that were automatically triggered:

pingcap/tidb/ghpr_build
pingcap/tidb/ghpr_check
pingcap/tidb/ghpr_check2
pingcap/tidb/ghpr_mysql_test
pingcap/tidb/ghpr_unit_test

In response to this:

/test tiprow_fast_test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

tangenta · 2023-09-01T04:31:05Z

disttask/framework/planner/planner.go

+type Planner struct{}
+
+// Run runs the distribute plan.
+func (*Planner) Run(planCtx PlanCtx, plan LogicalPlan) (int64, error) {


GMHDBJD · 2023-09-01T06:53:33Z

/retest

D3Hunter

5/13 review later

ti-chi-bot · 2023-09-04T06:06:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tangenta, ywqzzy

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [tangenta]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ti-chi-bot · 2023-09-04T06:06:55Z

[LGTM Timeline notifier]

Timeline:

2023-08-31 09:45:18.488400438 +0000 UTC m=+2006683.037416425: ☑️ agreed by ywqzzy.
2023-09-04 06:06:54.052178592 +0000 UTC m=+2339178.601194579: ☑️ agreed by tangenta.

GMHDBJD · 2023-09-04T07:14:15Z

/retest

GMHDBJD · 2023-09-04T09:54:20Z

/retest

tiprow · 2023-09-04T11:22:17Z

@GMHDBJD: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
tiprow_fast_test	`e51275b`	link	true	`/test tiprow_fast_test`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

GMHDBJD added 2 commits August 24, 2023 14:32

add planner

40ef59f

fix

e200895

ti-chi-bot bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 24, 2023

ywqzzy mentioned this pull request Aug 24, 2023

Enhance distributed task execution framework #46258

Closed

76 tasks

GMHDBJD added 2 commits August 24, 2023 16:08

add framework planner

69d8ed6

fix lint

70cf160

ti-chi-bot bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 24, 2023

GMHDBJD added 3 commits August 29, 2023 14:32

add ut

11b5de1

fix bazel

c14a836

add ut

2564090

ti-chi-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 29, 2023

ti-chi-bot bot added the ok-to-test Indicates a PR is ready to be tested. label Aug 29, 2023

ti-chi-bot bot requested a review from ywqzzy August 29, 2023 09:34

ywqzzy reviewed Aug 29, 2023

View reviewed changes

GMHDBJD added 2 commits August 29, 2023 18:01

update

d530616

fix ut

c90e10c

Benjamin2037 reviewed Aug 31, 2023

View reviewed changes

disttask/framework/planner/plan.go Show resolved Hide resolved

ywqzzy reviewed Aug 31, 2023

View reviewed changes

address comment

3d6a59e

ywqzzy approved these changes Aug 31, 2023

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Aug 31, 2023

tangenta reviewed Sep 1, 2023

View reviewed changes

GMHDBJD added 2 commits September 1, 2023 14:01

Merge remote-tracking branch 'upstream/master' into addPlanner

93ec35d

address comment

9778991

D3Hunter reviewed Sep 1, 2023

View reviewed changes

tangenta approved these changes Sep 4, 2023

View reviewed changes

ti-chi-bot bot added lgtm approved and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Sep 4, 2023

Merge remote-tracking branch 'upstream/master' into addPlanner

c64a464

update

bede27d

disable race

e51275b

ti-chi-bot bot merged commit 6707afa into pingcap:master Sep 4, 2023
5 of 9 checks passed

ywqzzy mentioned this pull request Sep 11, 2023

flaky test TestOwnerChange in disttask/framework pkg #46826

Closed


		type mockPlan struct{}

		func (mockPlan) ToTaskMeta() ([]byte, error) {

		store := testkit.CreateMockStore(t)
		gtk := testkit.NewTestKit(t, store)

distribute framework: add planner #46395

distribute framework: add planner #46395

Conversation

GMHDBJD commented Aug 24, 2023 • edited by Benjamin2037 Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

GMHDBJD commented Aug 24, 2023

tiprow bot commented Aug 24, 2023

codecov bot commented Aug 24, 2023 • edited Loading

Codecov Report

GMHDBJD commented Aug 29, 2023

GMHDBJD commented Aug 29, 2023

tiprow bot commented Aug 29, 2023

GMHDBJD commented Aug 29, 2023

GMHDBJD commented Aug 29, 2023

ti-chi-bot bot commented Aug 29, 2023

ywqzzy commented Aug 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GMHDBJD Aug 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GMHDBJD Aug 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GMHDBJD commented Aug 31, 2023

ywqzzy left a comment

Choose a reason for hiding this comment

GMHDBJD commented Aug 31, 2023

GMHDBJD commented Aug 31, 2023

ti-chi-bot bot commented Aug 31, 2023

GMHDBJD commented Aug 31, 2023

ti-chi-bot bot commented Aug 31, 2023

Choose a reason for hiding this comment

GMHDBJD commented Sep 1, 2023

D3Hunter left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Sep 4, 2023

ti-chi-bot bot commented Sep 4, 2023

[LGTM Timeline notifier]

GMHDBJD commented Sep 4, 2023

GMHDBJD commented Sep 4, 2023

tiprow bot commented Sep 4, 2023

GMHDBJD commented Aug 24, 2023 •

edited by Benjamin2037

Loading

codecov bot commented Aug 24, 2023 •

edited

Loading

GMHDBJD Aug 29, 2023 •

edited

Loading

GMHDBJD Aug 29, 2023 •

edited

Loading